Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to choose hostname and FQDN to be lowercased or not #236

Closed
wants to merge 5 commits into from

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Aug 22, 2024

Adds a provider option to choose to lowercase the hostname/FQDN

This addresses the behavior change introduced in v1.11.0 and reverted in v1.14.1. By default, hostnames are not lowercased.

Related issues

@AndersonQ AndersonQ added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Aug 22, 2024
@AndersonQ AndersonQ self-assigned this Aug 22, 2024
@AndersonQ AndersonQ force-pushed the set-lowerHostname branch 3 times, most recently from 0b70157 to 5167c16 Compare August 22, 2024 10:09
providers.LowercaseHostname and providers.SetLowerHostname allow to control hostname case sensitivity

This addresses the behavior change introduced in v1.11.0 and reverted in v1.14.1. By default, hostnames are not lowercased.
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this library should not handle this normalization for users. It's easy enough that any caller that wants normalization can do strings.ToLower on their own. I do however think types.HostInfo could more explicitly document that the value is passed-through without any normalization.

go-sysinfo has avoided any global state thus far because this was a problem we observed with similar libraries. If two usages in the same process have different needs then they end up stepping on each other when configuring the global SetLowerHostname. We should avoid this.

I have mentioned this in other places, but haven't opened an issue (will do). The providers/ packages should be treated as internal, and I would like to make this official by moving it to internal/providers in a "v2" at some point. This way the only API surface is the top-level API. This will give us a little more flexibility in refactoring and force users to go through the front door as it was designed. This would mean that the lowercase behavior must be configurable as a ProviderOption, but I don't think this is a necessary configuration option.

@AndersonQ
Copy link
Member Author

@andrewkroh fair point, that was a quick fix, I changed it now using ProviderOption

@@ -45,6 +45,12 @@ func WithHostFS(hostfs string) ProviderOption {
}
}

func WithLowerHostname() ProviderOption {
Copy link
Member

@andrewkroh andrewkroh Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to have some godoc explaining what it does (and what fields are affected) and what the default behavior is if you don't set the option.

@AndersonQ
Copy link
Member Author

I'll close it for now as we found another solution to upgrade elastic-agent-system-metrics for 8.15.1

@AndersonQ AndersonQ closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants